[NFC][Flang] Move bounds helper functions to Util header. #158658

TIFitis · 2025-09-15T15:20:09Z

This PR moves the needsBoundsOps and genBoundsOps helper functions to flang/include/flang/Optimizer/OpenMP/Utils.h.

Also moves flang/lib/Optimizer/Builder/BoxValue.cpp → flang/lib/Optimizer/Support/BoxValue.cpp to prevent build errors.

This PR moves the needsBoundsOps and genBoundsOps helper functions to flang/include/flang/Optimizer/OpenMP/Utils.h.

…upport/BoxValue.cpp.

llvmbot · 2025-09-15T15:20:42Z

@llvm/pr-subscribers-flang-fir-hlfir

Author: Akash Banerjee (TIFitis)

Changes

This PR moves the needsBoundsOps and genBoundsOps helper functions to flang/include/flang/Optimizer/OpenMP/Utils.h.

Also moves flang/lib/Optimizer/Builder/BoxValue.cpp → flang/lib/Optimizer/Support/BoxValue.cpp to prevent build errors.

Full diff: https://github.com/llvm/llvm-project/pull/158658.diff

7 Files Affected:

(modified) flang/include/flang/Optimizer/OpenMP/Utils.h (+40)
(modified) flang/lib/Lower/OpenMP/OpenMP.cpp (+5-6)
(modified) flang/lib/Optimizer/Builder/CMakeLists.txt (-1)
(modified) flang/lib/Optimizer/OpenMP/AutomapToTargetData.cpp (+3-32)
(modified) flang/lib/Optimizer/OpenMP/MapsForPrivatizedSymbols.cpp (+3-35)
(renamed) flang/lib/Optimizer/Support/BoxValue.cpp (-1)
(modified) flang/lib/Optimizer/Support/CMakeLists.txt (+1)

diff --git a/flang/include/flang/Optimizer/OpenMP/Utils.h b/flang/include/flang/Optimizer/OpenMP/Utils.h
index 636c768b016b7..235e667130659 100644
--- a/flang/include/flang/Optimizer/OpenMP/Utils.h
+++ b/flang/include/flang/Optimizer/OpenMP/Utils.h
@@ -13,6 +13,17 @@
 #ifndef FORTRAN_OPTIMIZER_OPENMP_UTILS_H
 #define FORTRAN_OPTIMIZER_OPENMP_UTILS_H
 
+#include "flang/Optimizer/Builder/BoxValue.h"
+#include "flang/Optimizer/Builder/DirectivesCommon.h"
+#include "flang/Optimizer/Builder/FIRBuilder.h"
+#include "flang/Optimizer/Builder/HLFIRTools.h"
+#include "flang/Optimizer/Dialect/FIRType.h"
+
+#include "mlir/Dialect/OpenMP/OpenMPDialect.h"
+#include "mlir/IR/Value.h"
+
+#include "llvm/ADT/SmallVector.h"
+
 namespace flangomp {
 
 enum class DoConcurrentMappingKind {
@@ -21,6 +32,35 @@ enum class DoConcurrentMappingKind {
   DCMK_Device ///< Lower to run in parallel on the GPU.
 };
 
+/// Return true if the variable has a dynamic size and therefore requires
+/// bounds operations to describe its extents.
+inline bool needsBoundsOps(mlir::Value var) {
+  assert(mlir::isa<mlir::omp::PointerLikeType>(var.getType()) &&
+         "needsBoundsOps can deal only with pointer types");
+  mlir::Type t = fir::unwrapRefType(var.getType());
+  if (mlir::Type inner = fir::dyn_cast_ptrOrBoxEleTy(t))
+    return fir::hasDynamicSize(inner);
+  return fir::hasDynamicSize(t);
+}
+
+/// Generate MapBoundsOp operations for the variable and append them to
+/// `boundsOps`.
+inline llvm::SmallVector<mlir::Value> genBoundsOps(fir::FirOpBuilder &builder,
+                                                   mlir::Value var,
+                                                   bool isAssumedSize = false,
+                                                   bool isOptional = false) {
+  mlir::Location loc = var.getLoc();
+  fir::factory::AddrAndBoundsInfo info =
+      fir::factory::getDataOperandBaseAddr(builder, var, isOptional, loc);
+  fir::ExtendedValue exv =
+      hlfir::translateToExtendedValue(loc, builder, hlfir::Entity{info.addr},
+                                      /*contiguousHint=*/true)
+          .first;
+  return fir::factory::genImplicitBoundsOps<mlir::omp::MapBoundsOp,
+                                            mlir::omp::MapBoundsType>(
+      builder, info, exv, isAssumedSize, loc);
+}
+
 } // namespace flangomp
 
 #endif // FORTRAN_OPTIMIZER_OPENMP_UTILS_H
diff --git a/flang/lib/Lower/OpenMP/OpenMP.cpp b/flang/lib/Lower/OpenMP/OpenMP.cpp
index def6cfff88231..add9326f665c2 100644
--- a/flang/lib/Lower/OpenMP/OpenMP.cpp
+++ b/flang/lib/Lower/OpenMP/OpenMP.cpp
@@ -30,6 +30,7 @@
 #include "flang/Optimizer/Builder/Todo.h"
 #include "flang/Optimizer/Dialect/FIRType.h"
 #include "flang/Optimizer/HLFIR/HLFIROps.h"
+#include "flang/Optimizer/OpenMP/Utils.h"
 #include "flang/Parser/characters.h"
 #include "flang/Parser/openmp-utils.h"
 #include "flang/Parser/parse-tree.h"
@@ -2495,12 +2496,10 @@ genTargetOp(lower::AbstractConverter &converter, lower::SymMap &symTable,
           Fortran::lower::getDataOperandBaseAddr(
               converter, firOpBuilder, sym.GetUltimate(),
               converter.getCurrentLocation());
-      llvm::SmallVector<mlir::Value> bounds =
-          fir::factory::genImplicitBoundsOps<mlir::omp::MapBoundsOp,
-                                             mlir::omp::MapBoundsType>(
-              firOpBuilder, info, dataExv,
-              semantics::IsAssumedSizeArray(sym.GetUltimate()),
-              converter.getCurrentLocation());
+      llvm::SmallVector<mlir::Value> bounds = flangomp::genBoundsOps(
+          firOpBuilder, info.rawInput,
+          semantics::IsAssumedSizeArray(sym.GetUltimate()),
+          semantics::IsOptional(sym.GetUltimate()));
       mlir::Value baseOp = info.rawInput;
       mlir::Type eleType = baseOp.getType();
       if (auto refType = mlir::dyn_cast<fir::ReferenceType>(baseOp.getType()))
diff --git a/flang/lib/Optimizer/Builder/CMakeLists.txt b/flang/lib/Optimizer/Builder/CMakeLists.txt
index 404afd185fd31..3a42c32a12f3c 100644
--- a/flang/lib/Optimizer/Builder/CMakeLists.txt
+++ b/flang/lib/Optimizer/Builder/CMakeLists.txt
@@ -2,7 +2,6 @@ get_property(dialect_libs GLOBAL PROPERTY MLIR_DIALECT_LIBS)
 get_property(extension_libs GLOBAL PROPERTY MLIR_EXTENSION_LIBS)
 
 add_flang_library(FIRBuilder
-  BoxValue.cpp
   Character.cpp
   Complex.cpp
   CUFCommon.cpp
diff --git a/flang/lib/Optimizer/OpenMP/AutomapToTargetData.cpp b/flang/lib/Optimizer/OpenMP/AutomapToTargetData.cpp
index 8b9991301aae8..d4be315c167be 100644
--- a/flang/lib/Optimizer/OpenMP/AutomapToTargetData.cpp
+++ b/flang/lib/Optimizer/OpenMP/AutomapToTargetData.cpp
@@ -13,6 +13,7 @@
 #include "flang/Optimizer/Dialect/FIRType.h"
 #include "flang/Optimizer/Dialect/Support/KindMapping.h"
 #include "flang/Optimizer/HLFIR/HLFIROps.h"
+#include "flang/Optimizer/OpenMP/Utils.h"
 
 #include "mlir/Dialect/OpenMP/OpenMPDialect.h"
 #include "mlir/Dialect/OpenMP/OpenMPInterfaces.h"
@@ -33,36 +34,6 @@ namespace {
 class AutomapToTargetDataPass
     : public flangomp::impl::AutomapToTargetDataPassBase<
           AutomapToTargetDataPass> {
-
-  // Returns true if the variable has a dynamic size and therefore requires
-  // bounds operations to describe its extents.
-  inline bool needsBoundsOps(mlir::Value var) {
-    assert(mlir::isa<mlir::omp::PointerLikeType>(var.getType()) &&
-           "only pointer like types expected");
-    mlir::Type t = fir::unwrapRefType(var.getType());
-    if (mlir::Type inner = fir::dyn_cast_ptrOrBoxEleTy(t))
-      return fir::hasDynamicSize(inner);
-    return fir::hasDynamicSize(t);
-  }
-
-  // Generate MapBoundsOp operations for the variable if required.
-  inline void genBoundsOps(fir::FirOpBuilder &builder, mlir::Value var,
-                           llvm::SmallVectorImpl<mlir::Value> &boundsOps) {
-    mlir::Location loc = var.getLoc();
-    fir::factory::AddrAndBoundsInfo info =
-        fir::factory::getDataOperandBaseAddr(builder, var,
-                                             /*isOptional=*/false, loc);
-    fir::ExtendedValue exv =
-        hlfir::translateToExtendedValue(loc, builder, hlfir::Entity{info.addr},
-                                        /*contiguousHint=*/true)
-            .first;
-    llvm::SmallVector<mlir::Value> tmp =
-        fir::factory::genImplicitBoundsOps<mlir::omp::MapBoundsOp,
-                                           mlir::omp::MapBoundsType>(
-            builder, info, exv, /*dataExvIsAssumedSize=*/false, loc);
-    llvm::append_range(boundsOps, tmp);
-  }
-
   void findRelatedAllocmemFreemem(fir::AddrOfOp addressOfOp,
                                   llvm::DenseSet<fir::StoreOp> &allocmems,
                                   llvm::DenseSet<fir::LoadOp> &freemems) {
@@ -112,8 +83,8 @@ class AutomapToTargetDataPass
     auto addMapInfo = [&](auto globalOp, auto memOp) {
       builder.setInsertionPointAfter(memOp);
       SmallVector<Value> bounds;
-      if (needsBoundsOps(memOp.getMemref()))
-        genBoundsOps(builder, memOp.getMemref(), bounds);
+      if (flangomp::needsBoundsOps(memOp.getMemref()))
+        bounds = flangomp::genBoundsOps(builder, memOp.getMemref());
 
       omp::TargetEnterExitUpdateDataOperands clauses;
       mlir::omp::MapInfoOp mapInfo = mlir::omp::MapInfoOp::create(
diff --git a/flang/lib/Optimizer/OpenMP/MapsForPrivatizedSymbols.cpp b/flang/lib/Optimizer/OpenMP/MapsForPrivatizedSymbols.cpp
index 30328573b74fc..5c1a3d232a8c9 100644
--- a/flang/lib/Optimizer/OpenMP/MapsForPrivatizedSymbols.cpp
+++ b/flang/lib/Optimizer/OpenMP/MapsForPrivatizedSymbols.cpp
@@ -29,6 +29,7 @@
 #include "flang/Optimizer/Dialect/Support/KindMapping.h"
 #include "flang/Optimizer/HLFIR/HLFIROps.h"
 #include "flang/Optimizer/OpenMP/Passes.h"
+#include "flang/Optimizer/OpenMP/Utils.h"
 
 #include "mlir/Dialect/Func/IR/FuncOps.h"
 #include "mlir/Dialect/OpenMP/OpenMPDialect.h"
@@ -106,8 +107,8 @@ class MapsForPrivatizedSymbolsPass
     // Figure out the bounds because knowing the bounds will help the subsequent
     // MapInfoFinalizationPass map the underlying data of the descriptor.
     llvm::SmallVector<mlir::Value> boundsOps;
-    if (needsBoundsOps(varPtr))
-      genBoundsOps(builder, varPtr, boundsOps);
+    if (flangomp::needsBoundsOps(varPtr))
+      boundsOps = flangomp::genBoundsOps(builder, varPtr);
 
     mlir::omp::VariableCaptureKind captureKind =
         mlir::omp::VariableCaptureKind::ByRef;
@@ -194,38 +195,5 @@ class MapsForPrivatizedSymbolsPass
       }
     }
   }
-  // As the name suggests, this function examines var to determine if
-  // it has dynamic size. If true, this pass'll have to extract these
-  // bounds from descriptor of var and add the bounds to the resultant
-  // MapInfoOp.
-  bool needsBoundsOps(mlir::Value var) {
-    assert(mlir::isa<omp::PointerLikeType>(var.getType()) &&
-           "needsBoundsOps can deal only with pointer types");
-    mlir::Type t = fir::unwrapRefType(var.getType());
-    // t could be a box, so look inside the box
-    auto innerType = fir::dyn_cast_ptrOrBoxEleTy(t);
-    if (innerType)
-      return fir::hasDynamicSize(innerType);
-    return fir::hasDynamicSize(t);
-  }
-
-  void genBoundsOps(fir::FirOpBuilder &builder, mlir::Value var,
-                    llvm::SmallVector<mlir::Value> &boundsOps) {
-    mlir::Location loc = var.getLoc();
-    fir::factory::AddrAndBoundsInfo info =
-        fir::factory::getDataOperandBaseAddr(builder, var,
-                                             /*isOptional=*/false, loc);
-    fir::ExtendedValue extendedValue =
-        hlfir::translateToExtendedValue(loc, builder, hlfir::Entity{info.addr},
-                                        /*continguousHint=*/true)
-            .first;
-    llvm::SmallVector<mlir::Value> boundsOpsVec =
-        fir::factory::genImplicitBoundsOps<mlir::omp::MapBoundsOp,
-                                           mlir::omp::MapBoundsType>(
-            builder, info, extendedValue,
-            /*dataExvIsAssumedSize=*/false, loc);
-    for (auto bounds : boundsOpsVec)
-      boundsOps.push_back(bounds);
-  }
 };
 } // namespace
diff --git a/flang/lib/Optimizer/Builder/BoxValue.cpp b/flang/lib/Optimizer/Support/BoxValue.cpp
similarity index 99%
rename from flang/lib/Optimizer/Builder/BoxValue.cpp
rename to flang/lib/Optimizer/Support/BoxValue.cpp
index a90ce5570de7d..2d814e6627599 100644
--- a/flang/lib/Optimizer/Builder/BoxValue.cpp
+++ b/flang/lib/Optimizer/Support/BoxValue.cpp
@@ -11,7 +11,6 @@
 //===----------------------------------------------------------------------===//
 
 #include "flang/Optimizer/Builder/BoxValue.h"
-#include "flang/Optimizer/Builder/FIRBuilder.h"
 #include "flang/Optimizer/Builder/Todo.h"
 #include "mlir/IR/BuiltinTypes.h"
 #include "llvm/Support/Debug.h"
diff --git a/flang/lib/Optimizer/Support/CMakeLists.txt b/flang/lib/Optimizer/Support/CMakeLists.txt
index 38038e1e9821d..825cf35173cc2 100644
--- a/flang/lib/Optimizer/Support/CMakeLists.txt
+++ b/flang/lib/Optimizer/Support/CMakeLists.txt
@@ -1,4 +1,5 @@
 add_flang_library(FIRSupport
+  BoxValue.cpp
   DataLayout.cpp
   InitFIR.cpp
   InternalNames.cpp

Copilot

Pull Request Overview

This PR moves helper functions needsBoundsOps and genBoundsOps to a centralized utility header and relocates BoxValue.cpp to resolve build dependencies.

Key changes:

Centralizes bounds helper functions in flang/include/flang/Optimizer/OpenMP/Utils.h
Moves BoxValue.cpp from Builder to Support library to prevent circular dependencies
Updates multiple OpenMP-related files to use the centralized utility functions

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
flang/include/flang/Optimizer/OpenMP/Utils.h	Adds `needsBoundsOps` and `genBoundsOps` helper functions to centralized header
flang/lib/Optimizer/Support/CMakeLists.txt	Adds `BoxValue.cpp` to Support library build
flang/lib/Optimizer/Support/BoxValue.cpp	Removes unused include after relocation
flang/lib/Optimizer/Builder/CMakeLists.txt	Removes `BoxValue.cpp` from Builder library build
flang/lib/Optimizer/OpenMP/MapsForPrivatizedSymbols.cpp	Updates to use centralized bounds functions and removes local implementations
flang/lib/Optimizer/OpenMP/AutomapToTargetData.cpp	Updates to use centralized bounds functions and removes local implementations
flang/lib/Lower/OpenMP/OpenMP.cpp	Updates to use centralized bounds generation function

Copilot · 2025-09-15T15:21:00Z

flang/lib/Optimizer/OpenMP/MapsForPrivatizedSymbols.cpp

+    if (flangomp::needsBoundsOps(varPtr))
+      boundsOps = flangomp::genBoundsOps(builder, varPtr);


The function call assignment changes the API contract. The old code appended to an existing vector via reference parameter, while the new code returns a new vector. This could impact performance if boundsOps already contains elements, as they would be overwritten rather than appended to.

flang/lib/Lower/OpenMP/OpenMP.cpp

TIFitis · 2025-09-15T15:22:23Z

@bhandarkar-pranav @skatrak I tried to merge #154164 but that still results in build errors. I've tried to get around the linker errors by adding an additional change of moving flang/lib/Optimizer/Builder/BoxValue.cpp → flang/lib/Optimizer/Support/BoxValue.cpp to this PR.

bhandarkar-pranav

@TIFitis thanks for this change. I have a more basic question though - Why did the undefined symbol to fir::operator<<(llvm::raw_ostream&, fir::ProcBoxValue const&) show up in the first place? Where or how was the circular dependency introduced?

bhandarkar-pranav · 2025-09-17T04:44:58Z

flang/include/flang/Optimizer/OpenMP/Utils.h

 #ifndef FORTRAN_OPTIMIZER_OPENMP_UTILS_H
 #define FORTRAN_OPTIMIZER_OPENMP_UTILS_H

+#include "flang/Optimizer/Builder/BoxValue.h"


Shouldn't BoxValue.h move to flang/Optimizer/Support?

TIFitis added 2 commits September 15, 2025 15:18

[NFC][Flang] Move bounds helper functions to Util header.

eed67c4

This PR moves the needsBoundsOps and genBoundsOps helper functions to flang/include/flang/Optimizer/OpenMP/Utils.h.

Move flang/lib/Optimizer/Builder/BoxValue.cpp → flang/lib/Optimizer/S…

a5e6279

…upport/BoxValue.cpp.

TIFitis requested review from skatrak, bhandarkar-pranav and Copilot September 15, 2025 15:20

llvmbot added flang Flang issues not falling into any other category flang:fir-hlfir labels Sep 15, 2025

Copilot AI reviewed Sep 15, 2025

View reviewed changes

bhandarkar-pranav reviewed Sep 17, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[NFC][Flang] Move bounds helper functions to Util header. #158658

[NFC][Flang] Move bounds helper functions to Util header. #158658

TIFitis commented Sep 15, 2025

Uh oh!

llvmbot commented Sep 15, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Sep 15, 2025

Uh oh!

Uh oh!

TIFitis commented Sep 15, 2025

Uh oh!

bhandarkar-pranav left a comment

Uh oh!

bhandarkar-pranav Sep 17, 2025

Uh oh!

Uh oh!

		if (flangomp::needsBoundsOps(varPtr))
		boundsOps = flangomp::genBoundsOps(builder, varPtr);

[NFC][Flang] Move bounds helper functions to Util header. #158658

Are you sure you want to change the base?

[NFC][Flang] Move bounds helper functions to Util header. #158658

Conversation

TIFitis commented Sep 15, 2025

Uh oh!

llvmbot commented Sep 15, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Sep 15, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

TIFitis commented Sep 15, 2025

Uh oh!

bhandarkar-pranav left a comment

Choose a reason for hiding this comment

Uh oh!

bhandarkar-pranav Sep 17, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!